-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[staging] perl: 5.38.2 -> 5.40.0 #333286
[staging] perl: 5.38.2 -> 5.40.0 #333286
Conversation
a38b264
to
ae6a50b
Compare
08e0920
to
bfd04de
Compare
93fbf32
to
6cc4272
Compare
- perl: perl538 -> perl540 - perl: disable parallel building as it fails for 5.40 - perlPackages: perl538Packages -> perl540Packages - perlCross: 84db4c7 -> 1.6
72ae686
to
816958c
Compare
Only provide one `withPerl` argument instead of one per perl version provided by nixpkgs.
e580156
to
67ecdcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Barring any objections, I'd like to merge this in the next day or so. |
|
||
- my @other = blessed($other) ? $other : @$other; | ||
+ eval 'require Scalar::Util'; | ||
+ my @other = unless($@){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcusramberg Hm, just came across this part - which seems broken? It's also in the original cross.patch
tho..
I don't like the idea of disabling parallelism because people spec their hardware disproportionally and/or configure their nix-daemon wrong. That should be reserved for cases where the build system has an actual bug due to parallelism that is independent of the build host. IIRC Make has a feature to limit the maximum load it can put on the system. Couldn't this be used instead? |
@Atemu I've tested building by setting a max value for diff --git a/pkgs/development/interpreters/perl/interpreter.nix b/pkgs/development/interpreters/perl/interpreter.nix
index b95adaf8287d..20364a145925 100644
--- a/pkgs/development/interpreters/perl/interpreter.nix
+++ b/pkgs/development/interpreters/perl/interpreter.nix
@@ -148,7 +148,7 @@ stdenv.mkDerivation (rec {
dontAddPrefix = !crossCompiling;
- enableParallelBuilding = false;
+ enableParallelBuilding = true;
# perl includes the build date, the uname of the build system and the
# username of the build user in some files.
@@ -210,6 +210,18 @@ stdenv.mkDerivation (rec {
perlOnTargetForTarget = if lib.hasAttr perlAttr pkgsTargetTarget then (override pkgsTargetTarget.${perlAttr}) else { };
};
+ buildPhase = ''
+ runHook preBuild
+
+ if (( $NIX_BUILD_CORES > 8)); then
+ NIX_BUILD_CORES=8
+ fi
+
+ make -j$NIX_BUILD_CORES
+
+ runHook postBuild
+ '';
+
doCheck = false; # some tests fail, expensive
# TODO: it seems like absolute paths to some coreutils is required. |
What was the hurry with merging this while there was discussion ongoing? We're still in the middle of staging-next-24.05, so this could absolutely have waited a couple of days to let the discussion play out. "Barring any objections" didn't seem to mean very much… |
While I'd prefer the issue of parallel builds to be discussed, it's a minor issue all things considered; we can live with single-threaded perl builds for a staging cycle or two. |
I agree that it's not a big deal. Just seemed odd to me to merge it while discussion was still happening when the next staging cycle won't start for days anyway. |
In the commit message, I wrote this:
I'm realizing that this message is not surfaced in the PR or that discoverable other than by hovering over the hash, so I'm committing to writing something on a PR if I do a merge commit message like that again. More generally, I'd like be part of the solution to a problem many, many, many contributors to Nixpkgs say they have: they don't know where the "finish line" is for merging their PR, and which feedback is blocking and which is just a general discussion or dissatisfaction about the state of the world. And so as a result as their limited capacity for discussion, or to keep up with the tax of keeping a PR fresh, many excellent contributions have died on the vine, despite no one in the PR process actually deciding to block the PR for a specific reason. That's not a worry here. Perl 5.40 is going to happen one way or another. So making this be the place to make a stand isn't the wisest decision I've ever made... What I'd like to do is help us, the collective who keeps Nixpkgs humming, become better about crisply stating objections by making an ask to contributors to do that and, failing to hear objections that state that they block the PR, merging it. I'm very open to feedback about how I do this, about the length of time for a debate or waiting for contributors to have their say. Definitely going to get this wrong some of the time. I'm much less open to practices that end up contributing to a culture of endless debate without clear messages like "I oppose merging this PR because it does XYZ, and XYZ shouldn't be done." The request changes button is right there, it's useful to ask people to click it when needed, otherwise, let's merge and iterate. Again, definitely might have gotten that wrong in this case. Based on @Atemu's message above, it sounds like they didn't consider their feedback blocking, so I do feel like it's a fair move to merge, but I am open to feedback about how I ought to behave in the future. |
Btw, we don't need to wait for the load limit PR to land. That PR is only about adding generic support that can be used tree-wide in a manner that fits all users. We can just set a load limit to a constant relative to the available CPU resources here right now and it'd be merely slightly suboptimal at worst in some edge-cases. A hellofalot better than disabling paralellism outright. |
Load limits were dropped across the board in #192447 because they make things worse for Hydra. I think patchwork solutions here are probably not a great fix, since it’s hard to pick a value that works well for a wide variety of systems, and since load limit looks at the total system load, not the load actually being used by a build. FWIW I support just enabling the parallel build again here in a separate PR since it doesn’t cause issues for Hydra and there are local workarounds. |
Yeah, I think that's exactly it. This is a great attitude to have — it just didn't really apply here. All I'd have liked to see being different was waiting a few days, since it's a staging PR anyway so all that matters is that it catches the next cycle. |
@emilazy my point is that disabling parallelism entirely is even worse for hydra. Making this tunable is only required for enabling load limits globally. It is not a requirement for applying it in targeted cases like this one. |
|
Hydra Jobset: https://hydra.nixos.org/jobset/nixpkgs/perl-updates
perl
to 5.40.0perl-cross
to 1.6perl536
pkgsStatic.perl
(Perl interpreter statically linked to Musl libc) #295608 from @tobimenableParallelBuilding=false
asunicore/mktables
step apparently consumes a lot of resources with large parallel builds, causing the build to fail sometimesperl538
in stdenv/linux/default.nix sinceperl540
fails to build in stage1Add a 👍 reaction to pull requests you find important.